Note regarding the new-api-needs-documentation label:

This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, to please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.

Tagging subscribers to this area: @maryamariyan
See info in area-owners.md if you want to be subscribed.

All CLA requirements met.

Also, could you please add tests for this added functionality?

CC @davidfowl

Is there a reason the ActivityLogScope implements IReadOnlyList?

I don't think it has to implement IReadOnlyList<T> and it can be just IEnumerable<KeyValuePair<string, object>>. I believe originally implemneted with IReadOnlyList<T> because JsonConsoleFormatter used to check explicitly for IReadOnlyList<T> but we changed that recently to check for IEnumerable<KeyValuePair<string, object>>.

Also, could you please add tests for this added functionality?

runtime/src/libraries/Microsoft.Extensions.Logging/tests/Common/LoggerFactoryTest.cs

Line 171 in cbb5b90

public void TestActivityIds(ActivityTrackingOptions options)

Of course. Its on my TODO-list.

Is there a reason the ActivityLogScope implements IReadOnlyList?

I don't think it has to implement IReadOnlyList<T> and it can be just IEnumerable<KeyValuePair<string, object>>. I believe originally implemneted with IReadOnlyList<T> because JsonConsoleFormatter used to check explicitly for IReadOnlyList<T> but we changed that recently to check for IEnumerable<KeyValuePair<string, object>>.

Okay. Should I change from IReadyOnlyList<T> to IEnumerable<T>? Note: This is potentially breaking because one could use GetCustomProperty in his code to get the the IReadOnlyList<T> by using the name __ActivityLogScope__.

Also, could you please add tests for this added functionality?

Of course. Its on my TODO-list.

Thank you.

Should I change from IReadyOnlyList to IEnumerable?

Not important to do so for now.

@msallin did you have a chance to address the remaining feedback so we can move on?

@msallin did you have a chance to address the remaining feedback so we can move on?

Wasn't able yet do run the unit tests locally and I'm sure they fail.
I will fix them tomorrow evening and mark the PR as ready for review as soon as I'm ready.

Wasn't able yet do run the unit tests locally and I'm sure they fail.

Let me know if you need help with that.

@msallin looks there is some errors https://helixre8s23ayyeko0k025g8.blob.core.windows.net/dotnet-runtime-refs-pull-48722-merge-5b4b06b341d4447e9f/Microsoft.Extensions.Logging.Tests/console.edeb4dfd.log?sv=2019-07-07&se=2021-03-22T19%3A14%3A10Z&sr=c&sp=rl&sig=dUgpUHJSMEj0keRHUAiXUeIVIrGKg4F81mABpiBDnEA%3D

Looks you missed updating the line

[19:54:26] info: Discovering: Microsoft.Extensions.Logging.Tests.dll (method display = ClassAndMethod, method display options = None)
[19:54:26] info: Discovered:  Microsoft.Extensions.Logging.Tests.dll (found 104 of 127 test cases)
[19:54:26] info: Starting:    Microsoft.Extensions.Logging.Tests.dll
[19:54:26] fail: [FAIL] Microsoft.Extensions.Logging.Test.LoggerFactoryTest.TestActivityTrackingOptionsTags
[19:54:26] info: System.ArgumentException : Tags is invalid ActivityTrackingOptions value. (Parameter 'options')
[19:54:26] info:    at Microsoft.Extensions.Logging.LoggerFactory..ctor(IEnumerable`1 providers, IOptionsMonitor`1 filterOption, IOptions`1 options)
[19:54:26] info:    at System.Reflection.RuntimeConstructorInfo.InternalInvoke(Object obj, Object[] parameters, Boolean wrapExceptions)
[19:54:26] info:    at System.Reflection.RuntimeConstructorInfo.DoInvoke(Object obj, BindingFlags invokeAttr, Binder binder, Object[] parameters, CultureInfo culture)
[19:54:26] info:    at System.Reflection.RuntimeConstructorInfo.Invoke(BindingFlags invokeAttr, Binder binder, Object[] parameters, CultureInfo culture)
[19:54:26] info:    at Microsoft.Extensions.DependencyInjection.ServiceLookup.CallSiteRuntimeResolver.VisitConstructor(ConstructorCallSite constructorCallSite, RuntimeResolverContext context)
[19:54:26] info:    at Microsoft.Extensions.DependencyInjection.ServiceLookup.CallSiteVisitor`2[[Microsoft.Extensions.DependencyInjection.ServiceLookup.RuntimeResolverContext, Microsoft.Extensions.DependencyInjection, Version=6.0.0.0, Culture=neutral, PublicKeyToken=adb9793829ddae60],[System.Object, System.Private.CoreLib, Version=6.0.0.0, Culture=neutral, PublicKeyToken=7cec85d7bea7798e]].VisitCallSiteMain(ServiceCallSite callSite, RuntimeResolverContext argument)

Here we go. Now I've something we can work with/discuss.

1. My current approach doesn't work because a Scope needs to support ToString()

This can be seen by looking at the test:

Or in the SimpleConsoleFormatter:

The JsonConsoleFormatter e.g. would behave correctly:

Either the SimpleConsoleFormatter needs to be adjusted to iterate and not call ToString() or we need to wrap the Tags & Baggages in an object and overwrite ToString().
Third option is to overwrite ToString() for the BaggageLinkedList and `TagsLinkedList"

2. Some things to define

What do we do if tag or baggage value is null?
I don't know if it makes sense to log smth like "myKey: "

What do we do if the key of a baggage or tag is empty (is it even possible)?
I need to check if one can even add this to an Activity.

Either the SimpleConsoleFormatter needs to be adjusted to iterate and not call ToString() or we need to wrap the Tags & Baggages in an object and overwrite ToString().
Third option is to overwrite ToString() for the BaggageLinkedList and `TagsLinkedList"

The other option which I am inclining to is to add the Tags and Baggage to ActivityLogScope and then format them in ToString there. We'll still always get the Tag and Baggage from the Activity object to ensure using the updated lists there.

What do we do if tag or baggage value is null?

I believe it is ok to produce empty string at that time.

What do we do if the key of a baggage or tag is empty (is it even possible)? I need to check if one can even add this to an Activity.

It is possible, at that time we'll produce empty string for the key.

As I suggested, if we move the formatting logic to ActivityLogScope we can easily control the behavior.

The other option which I am inclining to is to add the Tags and Baggage to ActivityLogScope and then format them in ToString there. We'll still always get the Tag and Baggage from the Activity object to ensure using the updated lists there.

We have the activity as ctor param. You mean I should create a field and hold the Activity in the ActivityLogScope?
As a consquence the current baggage and tags can be applied in ToString(). The GetEnumerator will yield return _activity.TagObjects and baggage. But what about the access by Index and count?

Regarding the debugger output of activity accessing, it looks it is using the activity you have created which make sense. But still, something unclear to me.

Execute AdditionalActivityItemsLogScope ScopeActivity(|efff3b4b-4661424b2ed3102b.)
Instance was null ScopeActivity(|efff3b4b-4661424b2ed3102b.)

These two lines means the object AdditionalActivityItemsLogScope is created using the value activity.Baggage and then cached inside the activity object. then the following lines:

Execute AdditionalActivityItemsLogScope ScopeActivity(|efff3b4b-4661424b2ed3102b.)
Execute AdditionalActivityItemsLogScope ScopeActivity(|efff3b4b-4661424b2ed3102b.)
Execute AdditionalActivityItemsLogScope ScopeActivity(|efff3b4b-4661424b2ed3102b.)

means the cached value of AdditionalActivityItemsLogScope is retrieved and used in the logging. But this value is still holding the original IEnumerable value returned from activity.Baggage and that value shouldn't be including the other added key-values since we cached this object. The question, how the logs got the other key-value pairs added to the activity.Baggage while we used the old-cached value? Looks I am missing something in the middle here.

We don't cache the values. We always reenumerate the IEnumerable. This would cache the values:
additionalActivityItemsLogScope = new AdditionalActivityItemsLogScope<TValue>(items.ToList());

We don't cache the values. We always reenumerate the IEnumerable. This would cache the values:

You are right. I was assuming Activity.Baggage internally returning a list but it looks not.

@msallin please let me know when you address the remaining comments and we'll be good to go. thanks!

@tarekgh I think there aren't any comments left.

@msallin sorry it was my fault that I wrote the comments and didn't submit it. I posted them now. Thanks!

@msallin sorry it was my fault that I wrote the comments and didn't submit it. I posted them now. Thanks!

Great! Ty! Consider them done until the end of this week.

@tarekgh I addressed all the comments. You might have a look again at the PR again and let me know if there is something I should rework.

@msallin looks good to me. I have added one last minor comment and we'll be good to merge.

@msallin

##[error]src/libraries/Microsoft.Extensions.Logging/src/LoggerFactoryScopeProvider.cs(62,93): error SA1028: (NETCORE_ENGINEERING_TELEMETRY=Build) Code should not contain trailing whitespace

@msallin thanks a lot for helping with this issue.